-
Notifications
You must be signed in to change notification settings - Fork 807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Write the detect cycles function as a function to optimize code #1013
Conversation
wdfk-prog
commented
Aug 1, 2024
•
edited
Loading
edited
- Using the master branch code, the footprint size of the gcc compilation under Windows is as follows
- After this change, the compilation footprint is as follows
- The optimization result can be obtained,Compiles without error or warning.
- gcc version
Also worth mentioning is the use of gcc and makefiles on Windows; Can only compile and clear, view the overall occupancy size and generate assembly files, other operations can not take effect, will give an error; Is there any plan to support this part on Windows. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minor style stuff aside, I think there's a slight problem with that algorithm as it changes behaviour by moving the initialization part into the loop. This might still find trivial cases of loops, but will hide loops, that need the iteration behaviour of advancing the tortoises. You can check this by noticing that several of the assignments of the function lfs_detect_cycles
are now dead stores, when previously they would be re-used in the next loop iteration. Nonetheless, centralizing this check pattern may allow code size reduction.
To fix that bug, the following should be done:
- Introduce a structure to hold the state of the algorithm
typedef struct lfs_tortoise {
lfs_block_t block[2]; // Previously tortoise
lfs_size_t i; // Previously tortoise_i
lfs_size_t period; // Previously tortoise_period
} lfs_tortoise_t;
-
Pass a
lfs_tortoise_t*
as the second argument tolfs_detect_cycles
-
On the call sites: Outside the loop initialize a
lfs_tortoise_t
like so:
lfs_tortoise_t tortoise = {
.block = {LFS_BLOCK_NULL, LFS_BLOCK_NULL},
.i = 1,
.period = 1,
};
- Pass that variable to
lfs_detect_cycles
by reference.
The "cancelled" CI jobs were likely due to some loop in the filesystem under test not being detected after your change.
Thank you for reminding me that I made a stupid mistake |
dddbe2f
to
7cb6a79
Compare
lfs.c
Outdated
struct lfs_tortoise_t tortoise = { | ||
.pair = {LFS_BLOCK_NULL, LFS_BLOCK_NULL}, | ||
.i = 1, | ||
.period = 1, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to go outside the loop. Similar for the other 3 places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o(╥﹏╥)o,Sorry, I was so careless.
7cb6a79
to
0e8f4d7
Compare
Tests passed ✓, Code: 17096 B (+0.2%), Stack: 1448 B (+0.6%), Structs: 812 B (+0.0%)
|
lfs.c
Outdated
lfs_size_t period; | ||
}; | ||
|
||
static int lfs_detect_cycles(lfs_mdir_t dir, struct lfs_tortoise_t *tortoise) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you're passing the dir
by value, instead of by pointer. This creates a copy of the 8-word struct, which might be adding both stack cost and code cost.
Passing by pointer:
static int lfs_detect_cycles(const lfs_mdir_t *dir, struct lfs_tortoise_t *tortoise) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed it, it really shouldn't be, ^_^.
I've resubmitted it.
Unfortunately Windows is a difficult platform to support as a development environment. It's just not worth supporting vs leveraging all of the useful tools that are available in Linux. The best option would be use either a virtual machine or WSL to run a Linux image on windows and develop inside that. I've heard good things about WSL but haven't used it myself. |
0e8f4d7
to
ea53106
Compare
Sorry, this failure was an issue on our side. GitHub made a breaking change to their APIs recently. To fix the pr this needs to be rebased onto the devel branch: https://github.com/littlefs-project/littlefs/tree/devel, let me know if you want me to do that for you: $ git rebase origin/devel |
OK |
Sorry again, I was having problems pushing to your branch and couldn't understand why. You set "Maintainers are allowed to edit this pull request." correctly but pushes are still being rejected for some reason. And wow, I think this is a real mess up on GitHub's part. There is apparently a bug where cross-organization forks can't be edited by maintainers, and I think this might also be affecting forks-of-cross-org-forks: So unfortunately it looks like you either need to add me as a collaborator or do the rebase locally... Fortunately it's a simple rebase with no conflicts. |
|
Tests passed ✓, Code: 17000 B (-0.9%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
|
Looks good here, thanks for that. I've manually updated the bot comment since it's still a bit broken on our end. |
I left some review comments, mostly related to style/consistency with the rest of the codebase. But this looks good to me and we can probably bring it in on the next minor release. |
13bb02e
to
a2c2e49
Compare
|
Tests passed ✓, Code: 17004 B, Stack: 1440 B, Structs: 812 B
|
Thanks for making those changes. This looks good to me and will bring it in next release.
Point taken, it is a waste of both your time and my time commenting on style inconsistencies.
I had looked into astyle before, but struggled to make it do what I wanted. It was very opinionated about unnecessary things. But a custom python script is a good idea, maybe that's the way to go. |
Merging. Thanks again for the PR! And sorry about the rough process. |